Skip to content

Add cleanup IO process after nerdctl stop #4316

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add cleanup IO process after nerdctl stop #4316

wants to merge 1 commit into from

Conversation

liuchangyan
Copy link

Fix #4315
In the ctr and CRI call paths of containerd, the remaining FIFO files (e.g., /run/containerd/fifo) are primarily cleaned up through the Delete interface of the task. However, in the nerdctl stop path, this interface is not invoked, so an explicit call to io.Close is required to clean them up.

@liuchangyan liuchangyan mentioned this pull request Jun 13, 2025
Open
@fahedouch
Copy link
Member

can we have a test please ?

@liuchangyan
Copy link
Author

can we have a test please ?

Sure

@liuchangyan liuchangyan force-pushed the main branch 2 times, most recently from 87c6d21 to 2689504 Compare June 19, 2025 07:14
fahedouch
fahedouch requested changes Jun 20, 2025
View reviewed changes
cmd/nerdctl/container/container_stop_linux_test.go Outdated
// Stop the container after 2 seconds
go func() {
time.Sleep(2 * time.Second)
base.Cmd("stop", testContainerName).Run()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sequentially stopping the container makes things more deterministic. AssertOk here is necessary to ensure proper stop

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sequentially stopping the container makes things more deterministic. AssertOk here is necessary to ensure proper stop

Thanks! Since the container is long-running with --rm, we use a goroutine to trigger stop — otherwise run blocks and we can’t proceed. Already updated the stop call to use AssertOK to ensure it completes properly.

@apostasie
Copy link
Contributor

Hey @liuchangyan

Looks like the new tests are failing?

@ningmingxiao
Copy link
Contributor

I can help to create a ci for this bug.

@ningmingxiao ningmingxiao mentioned this pull request Aug 5, 2025
Open
@fahedouch
Copy link
Member

I can help to create a ci for this bug.

@ningmingxiao please go ahead

@ningmingxiao
Copy link
Contributor

ningmingxiao commented Aug 6, 2025 •
edited
Loading

I think we should mv io.close in waitContainerStop it will make sure container is really stopped.
I create a pr #4443

func waitContainerStop(ctx context.Context, task containerd.Task, exitCh <-chan containerd.ExitStatus, id string) error {
	select {
	case <-ctx.Done():
		if err := ctx.Err(); err != nil {
			return fmt.Errorf("wait container %v: %w", id, err)
		}
		return nil
	case status := <-exitCh:
		// Cleanup the IO after a successful Stop
		if io := task.IO(); io != nil {
			if cerr := io.Close(); cerr != nil {
				log.G(ctx).Warnf("failed to close IO for container %s: %v", id, cerr)
			}
		}
		return status.Error()
	}
}

@fahedouch
Copy link
Member

Hi @liuchangyan,

What is the next step for this PR?

@liuchangyan
Copy link
Author

liuchangyan commented Aug 13, 2025 •
edited
Loading

Hi @liuchangyan,

What is the next step for this PR?
I've removed my test sample since it looks like this PR4443 already covers that part. I think my original code still has value to keep. Should we merge both PRs together~ WDYT?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fahedouch fahedouch fahedouch requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

nerdctl stop could not cleanup fifo files(/run/containerd/fifo)
4 participants
@liuchangyan @fahedouch @apostasie @ningmingxiao